feat: add --app flag support to slack create for linking existing apps#565
feat: add --app flag support to slack create for linking existing apps#565srtaalej wants to merge 4 commits into
Conversation
srtaalej
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
- Coverage 71.68% 71.63% -0.06%
==========================================
Files 226 227 +1
Lines 19140 19261 +121
==========================================
+ Hits 13721 13797 +76
- Misses 4214 4245 +31
- Partials 1205 1219 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Awesome changes going on here! 🎁
I'm leaving a handful of comments around refactoring logic into adjacent places. Hoping that we can compose commands overall and avoid adding too much to create for ongoing iteration.
Two notable changes to the experience that I'll call out here include:
- Accepting the environment flag alongside a default
- Skipping the name prompt when an existing app ID is provided
Quite excited for what this hopes to unlock 🔏
| // fetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | ||
| func fetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | ||
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | ||
| if err != nil { | ||
| return types.SlackYaml{}, slackerror.New(slackerror.ErrInvalidManifest). | ||
| WithMessage("Failed to fetch manifest for app %s", appID) | ||
| } | ||
| return manifest, nil | ||
| } |
There was a problem hiding this comment.
| // fetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | |
| func fetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | |
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | |
| if err != nil { | |
| return types.SlackYaml{}, slackerror.New(slackerror.ErrInvalidManifest). | |
| WithMessage("Failed to fetch manifest for app %s", appID) | |
| } | |
| return manifest, nil | |
| } |
🪓 suggestion: Let's inline this! I think the error returned might sometimes be different from invalid manifest that we might want to surface
| // writeManifestToProject writes the fetched manifest JSON to the project directory. | ||
| func writeManifestToProject(fs afero.Fs, projectPath string, manifest types.SlackYaml) error { | ||
| manifestData, err := json.MarshalIndent(manifest.AppManifest, "", " ") | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to serialize app manifest") | ||
| } | ||
|
|
||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| if err := afero.WriteFile(fs, manifestPath, append(manifestData, '\n'), 0644); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to write manifest to project") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛻 suggestion: Let's move this logic to internal/app/manifest for adjacent changes of #543
| if appFlagProvided { | ||
| absProjectPath, err := filepath.Abs(appDirPath) | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| if nameFlagProvided { | ||
| remoteManifest.DisplayInformation.Name = displayName | ||
| } | ||
| if err := writeManifestToProject(clients.Fs, absProjectPath, remoteManifest); err != nil { | ||
| return err | ||
| } | ||
| // linkAppToProject requires the working directory to be the project | ||
| // because SaveDeployed/SaveLocal use os.Getwd() to find .slack/ | ||
| originalDir, _ := clients.Os.Getwd() | ||
| if err := os.Chdir(absProjectPath); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| linkErr := linkAppToProject(ctx, clients, appAuth, clients.Config.AppFlag, remoteManifest) | ||
| _ = os.Chdir(originalDir) | ||
| if linkErr != nil { | ||
| return linkErr | ||
| } | ||
| } |
There was a problem hiding this comment.
🧊 suggestion: We might want to instead share existing logic of internal package here and add an AppID to the "CreateArgs" above? I'd keep the cmd portion limited to input and output perhaps-
slack-cli/internal/pkg/create/create.go
Lines 149 to 161 in 9a8cbee
There was a problem hiding this comment.
you're right, this should def go in internal!
There was a problem hiding this comment.
🐌 question: Was this still something to move into internal? I'm hoping we might preferred the deferred chdir existing!
| // linkAppToProject saves the app to the project's apps JSON file. | ||
| // Defaults to local/dev unless the manifest explicitly uses a hosted runtime. | ||
| func linkAppToProject(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, appID string, manifest types.SlackYaml) error { | ||
| app := types.App{ | ||
| AppID: appID, | ||
| TeamID: auth.TeamID, | ||
| TeamDomain: auth.TeamDomain, | ||
| EnterpriseID: auth.EnterpriseID, | ||
| } | ||
|
|
||
| if manifest.IsFunctionRuntimeSlackHosted() { | ||
| return clients.AppClient().SaveDeployed(ctx, app) | ||
| } | ||
| app.IsDev = true | ||
| app.UserID = auth.UserID | ||
| return clients.AppClient().SaveLocal(ctx, app) | ||
| } |
There was a problem hiding this comment.
🔭 question: Can we reuse logic of the app link command? We perhaps might change outputs but I'm hoping we move toward focused and atomic commands that perhaps compose!
There was a problem hiding this comment.
👾 issue: I'm concerned of the forced default local app here. The same CI example I share earlier is for a production app and I don't have immediate option to "deploy" the right app after using this:
$ slack create --app A0582JYKGB1 --template zimeg/slacks --branch snaek --name snaek --force
🌠 suggestion: We might want to use the --environment flag to decide this? I still think a default "local" makes sense - CI should be explicit!
| WithMessage("The --app flag requires the --template flag when used with create") | ||
| } | ||
|
|
||
| // Fail fast: resolve auth and fetch manifest before creating the project |
There was a problem hiding this comment.
🪬 thought: Related to comments of logic moved to internal I'm curious if we can move this check too? I understand a mismatched app ID will cause error but I don't think we should prompt for name when the --app flag is used...
🐮 ramble: For example a minimal example seems excessive for CI use case:
$ slack create --app A0582JYKGB1 --template zimeg/slacks --branch snaek --name snaek --force
There was a problem hiding this comment.
🧂 ramble: Similar comment as below I wonder if this is logic can be passed as createArgs or perhaps afterwards to the link command?
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Leaving a few more comments around logic that we might want to separate between project create and app link command 🔭
To me it's seeming more that these commands should be sequenced so the --app and --environment flags are passed through the "link" command after the "create" command clones the template. I'm unsure of right scope for this but notice a few improvements happening:
- Finding saved authentication for a provided
linkapp 🎁 - Copying the existing app manifest from upstream app settings ⚙️
- Saving the provided app ID from
createflags 🏁
Am requesting approval to hope we can reuse more command logic and think these enhancements might be alright to break into multiple changesets if the notes above seem right?
| WithMessage("The --subdir flag requires the --template flag") | ||
| } | ||
|
|
||
| // --app requires --template (Mode 2 deferred) |
There was a problem hiding this comment.
👁️🗨️question: Which mode 2 in this case?
| if appFlagProvided { | ||
| absProjectPath, err := filepath.Abs(appDirPath) | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| if nameFlagProvided { | ||
| remoteManifest.DisplayInformation.Name = displayName | ||
| } | ||
| if err := writeManifestToProject(clients.Fs, absProjectPath, remoteManifest); err != nil { | ||
| return err | ||
| } | ||
| // linkAppToProject requires the working directory to be the project | ||
| // because SaveDeployed/SaveLocal use os.Getwd() to find .slack/ | ||
| originalDir, _ := clients.Os.Getwd() | ||
| if err := os.Chdir(absProjectPath); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| linkErr := linkAppToProject(ctx, clients, appAuth, clients.Config.AppFlag, remoteManifest) | ||
| _ = os.Chdir(originalDir) | ||
| if linkErr != nil { | ||
| return linkErr | ||
| } | ||
| } |
There was a problem hiding this comment.
🐌 question: Was this still something to move into internal? I'm hoping we might preferred the deferred chdir existing!
| // FetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | ||
| func FetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | ||
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | ||
| if err != nil { | ||
| return types.SlackYaml{}, slackerror.Wrap(err, slackerror.ErrInvalidManifest). | ||
| WithMessage("Failed to fetch manifest for app %s", appID) | ||
| } | ||
| return manifest, nil | ||
| } |
There was a problem hiding this comment.
🪓 suggestion: Let's inline this! I think the error returned might sometimes be different from invalid manifest that we might want to surface
| // FetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | |
| func FetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | |
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | |
| if err != nil { | |
| return types.SlackYaml{}, slackerror.Wrap(err, slackerror.ErrInvalidManifest). | |
| WithMessage("Failed to fetch manifest for app %s", appID) | |
| } | |
| return manifest, nil | |
| } |
| // SaveAppToProject writes the linked app to the project's apps JSON file, | ||
| // checking for conflicts before saving unless --force is set. | ||
| func SaveAppToProject(ctx context.Context, clients *shared.ClientFactory, app types.App) error { | ||
| deploy, err := clients.AppClient().GetDeployed(ctx, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| local, err := clients.AppClient().GetLocal(ctx, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| switch app.IsDev { | ||
| case true: | ||
| if clients.Config.ForceFlag || (local.IsNew() && deploy.AppID != app.AppID) { | ||
| return clients.AppClient().SaveLocal(ctx, app) | ||
| } | ||
| case false: | ||
| if clients.Config.ForceFlag || (deploy.IsNew() && local.AppID != app.AppID) { | ||
| return clients.AppClient().SaveDeployed(ctx, app) | ||
| } | ||
| } | ||
| return slackerror.New(slackerror.ErrAppFound). | ||
| WithMessage("A saved app was found and cannot be overwritten"). | ||
| WithRemediation("Remove the app from this project or try again with %s", style.Bold("--force")) | ||
| } |
There was a problem hiding this comment.
🦠 suggestion: This is duplicate to app link implementation we might not want to duplicate this?
Lines 283 to 307 in dc30dc3
| // WriteManifestToProject writes the fetched manifest JSON to the project directory. | ||
| func WriteManifestToProject(fs afero.Fs, projectPath string, manifest types.SlackYaml) error { | ||
| manifestData, err := json.MarshalIndent(manifest.AppManifest, "", " ") | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to serialize app manifest") | ||
| } | ||
|
|
||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| if err := afero.WriteFile(fs, manifestPath, append(manifestData, '\n'), 0644); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to write manifest to project") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗳️ suggestion: We might want to make this internal/app/manifest.go I think we want to move away from internal/pkg/....... ongoing
| // ResolveAuthForApp finds an authenticated workspace that has access to the given app ID. | ||
| func ResolveAuthForApp(ctx context.Context, clients *shared.ClientFactory, appID string) (types.SlackAuth, error) { | ||
| if clients.Config.TokenFlag != "" { | ||
| auth, err := clients.Auth().AuthWithToken(ctx, clients.Config.TokenFlag) | ||
| if err != nil { | ||
| return types.SlackAuth{}, slackerror.Wrap(err, slackerror.ErrNotAuthed) | ||
| } | ||
| return auth, nil | ||
| } | ||
|
|
||
| allAuths, err := clients.Auth().Auths(ctx) | ||
| if err != nil { | ||
| return types.SlackAuth{}, slackerror.Wrap(err, slackerror.ErrNotAuthed) | ||
| } | ||
|
|
||
| if len(allAuths) == 0 { | ||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrNotAuthed). | ||
| WithMessage("No workspaces connected"). | ||
| WithRemediation("Run %s to sign in to a workspace that has access to app %s", style.Commandf("login", false), appID) | ||
| } | ||
|
|
||
| if clients.Config.TeamFlag != "" { | ||
| for i := range allAuths { | ||
| if allAuths[i].TeamID == clients.Config.TeamFlag || allAuths[i].TeamDomain == clients.Config.TeamFlag { | ||
| if _, err := clients.API().GetAppStatus(ctx, allAuths[i].Token, []string{appID}, allAuths[i].TeamID); err == nil { | ||
| return allAuths[i], nil | ||
| } | ||
| } | ||
| } | ||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrTeamNotFound). | ||
| WithMessage("The specified team does not have access to app %s", appID). | ||
| WithRemediation("Run %s to sign in to the workspace that owns this app", style.Commandf("login", false)) | ||
| } | ||
|
|
||
| for i := range allAuths { | ||
| if _, err := clients.API().GetAppStatus(ctx, allAuths[i].Token, []string{appID}, allAuths[i].TeamID); err == nil { | ||
| return allAuths[i], nil | ||
| } | ||
| } | ||
|
|
||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrAppNotFound). | ||
| WithMessage("No authenticated workspace has access to app %s", appID). | ||
| WithRemediation("Run %s to sign in to the workspace that owns this app", style.Commandf("login", false)) | ||
| } |
There was a problem hiding this comment.
💾 thought: This might be something we use to replace the following:
Lines 211 to 240 in aceb7a4
There was a problem hiding this comment.
👾 ramble: Am leaving comments in hopes that these additions can be reused more as:
- Run the
createcommand- If "--app" flag then run the
linkcommand - No "--app" flag continues without change
- If "--app" flag then run the
| WithMessage("The --app flag requires the --template flag when used with create") | ||
| } | ||
|
|
||
| // Fail fast: resolve auth and fetch manifest before creating the project |
There was a problem hiding this comment.
🧂 ramble: Similar comment as below I wonder if this is logic can be passed as createArgs or perhaps afterwards to the link command?
2 participants
Changelog
slack createnow accepts the--appflag alongside--templateto scaffold a project and automatically link it to an existing app by fetching its manifest from App Settings.Summary
This PR adds
--app [ID]support toslack create. When used with--template, the CLI will:apps.manifest.export)manifest.jsonThe
--nameflag takes precedence over the remote manifest's display name when both are provided.Using
--appwithout--templatereturns an error with guidance.Example:
slack create my-project -t slack-samples/bolt-js-starter-template --app A0123456789Preview
📂 Project Create Cloning template slack-samples/bolt-js-starter-template To path ~/programming/slack-cli/my-project 📦 Project Dependencies Added my-project/.slack Added my-project/.slack/.gitignore Added my-project/.slack/config.json Added my-project/.slack/hooks.json Updated app manifest source to "project" (local) Added package @slack/cli-hooks@1.3.2 Installed dependencies using npm install 📋 Next Steps Learn more about the project in the README.md Change into your project with cd my-project/ Start developing and see changes in real-time with slack runTesting
Manual verification:
./bin/slack create my-project -t slack-samples/bolt-js-starter-template --app <real-app-id>.slack/apps.dev.jsoncontains linked app with correct team/app IDsmanifest.jsonmatches remote app's manifest--appwithout--templatereturns helpful error--nameoverrides manifest display name when providedNotes
manifest.json. A future PR will implement git-style merging where template and remote manifestsare combined.
Requirements